Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] Stop using function dependencies in propagateScopeDeps #31200

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Oct 11, 2024

Copy link

vercel bot commented Oct 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2024 7:27pm

Extends #31066 to ObjectMethods (somehow missed this before).

'
`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths

This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions

'
`enablePropagateScopeDepsHIR` is now used extensively in Meta. This has been tested for over two weeks in our e2e tests and production.

The rest of this stack deletes `LoweredFunction.dependencies`, which the non-hir version of `PropagateScopeDeps` depends on. To avoid a more forked HIR (non-hir with dependencies and hir with no dependencies), let's go ahead and clean up the non-hir version of PropagateScopeDepsHIR.

Note that all fixture changes in this PR were previously reviewed when they were copied to `propagate-scope-deps-hir-fork`. Will clean up / merge these duplicate fixtures in a later PR

'
Recursively collect identifier / property loads and optional chains from inner functions. This PR is in preparation for #31200

Previously, we only did this in `collectHoistablePropertyLoads` to understand hoistable property loads from inner functions.
1. collectTemporariesSidemap
2. collectOptionalChainSidemap
3. collectHoistablePropertyLoads
    - ^ this recursively calls `collectTemporariesSidemap`, `collectOptionalChainSidemap`, and `collectOptionalChainSidemap` on inner functions
4. collectDependencies

Now, we have
1. collectTemporariesSidemap
    - recursively record identifiers in inner functions. Note that we track all temporaries in the same map as `IdentifierIds` are currently unique across functions
2. collectOptionalChainSidemap
    - recursively records optional chain sidemaps in inner functions
3. collectHoistablePropertyLoads
    - (unchanged, except to remove recursive collection of temporaries)
4. collectDependencies
    - unchanged: to be modified to recursively collect dependencies in next PR

'
Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly.

This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204).

Some nice side effects
- optional-chaining deps for inner functions
- full DCE and outlining for inner functions (see #31202)
- fewer extraneous instructions (see #31204)

-
@@ -230,6 +230,8 @@ const EnvironmentConfigSchema = z.object({
*/
enableUseTypeAnnotations: z.boolean().default(false),

enableFunctionDependencyRewrite: z.boolean().default(true),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is deleted in #31204

const maybeOptionalChain = temporaries.get(operand[1].identifier.id);
if (maybeOptionalChain) {
context.visitDependency(maybeOptionalChain);
const handleFunction = (fn: HIRFunction): void => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is code movement (moving logic into handleFunction) so that we can recursively visit nested functions

Comment on lines +705 to +707
context.inInnerFn = true;
handleFunction(instr.value.loweredFunc.func);
context.inInnerFn = wasInInnerFn;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recursive call. See explanation in Context.#declare for why inInnerFn is needed

}
const y = t0;
const $ = _c(2);
const y = { b };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const y = { b } is dce'd in #31204 (after we delete LoweredFunction.dependencies)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants